Conversation
| let fieldName = generateFieldName(length, charset); | ||
| while (!canNameBeUsedInImport(fieldName)) | ||
| { | ||
| fieldName = generateFieldName(); |
There was a problem hiding this comment.
Should this be passed length and charset as well?
| Set<String> dataCols = new CaseInsensitiveHashSet(_rootTable.getColumnNameSet()); | ||
|
|
||
| // all columns from dataclass property table except name, lsid, and classid | ||
| // all columns from dataclass property table except name, lsid |
There was a problem hiding this comment.
nit: Consider changing this comment so it does not also need to enumerate what is changing. Something like:
// All columns from dataclass property table except key columns| // pass | ||
| } | ||
| if (null == kind || null == kind.getStorageSchemaName()) | ||
| return; |
There was a problem hiding this comment.
Log a message here stating that the kind or storage schema could not be resolved before returning.
| * Drop the classid column and add a rowId column to existing provisioned DataClass tables. | ||
| */ | ||
| @SuppressWarnings("unused") | ||
| @DeferredUpgrade |
There was a problem hiding this comment.
Just curious, does this script need to be deferred?
| // Set NOT NULL constraint | ||
| SqlExecutor executor = new SqlExecutor(scope); | ||
| boolean isPostgreSQL = scope.getSqlDialect().isPostgreSQL(); | ||
| if (isPostgreSQL) |
There was a problem hiding this comment.
nit: Consider using the table for these statements:
// Set NOT NULL constraint
SqlExecutor executor = new SqlExecutor(scope);
if (scope.getSqlDialect().isPostgreSQL())
executor.execute(new SQLFragment("ALTER TABLE ").append(provisionedTable).append(" ALTER COLUMN rowId SET NOT NULL"));
else
executor.execute(new SQLFragment("ALTER TABLE ").append(provisionedTable).append(" ALTER COLUMN rowId INT NOT NULL"));Same for the FK constraint call.
| // update description and fieldName value from Import with update, the import columns contains name field, verify update is successful and data are updated correctly | ||
| const importUpdateText = 'Name\tDescription\t' + fieldName + '\n' + newName3 + '\timportUpd1\timportVal1\n' + newName4 + '\timportUpd2\timportVal2'; | ||
| const updateResp = await ExperimentCRUDUtils.importData(server, importUpdateText, dataType, 'UPDATE', topFolderOptions, editorUserOptions); | ||
| expect(updateResp.text.indexOf('"success" : true') > -1).toBeTruthy(); |
There was a problem hiding this comment.
nit: Consider using the response body instead.
expect(updateResp.body.success).toBe(true);Same for the mergeResp below.
| // insert 2 rows data, provide explicit names and a rowId = 1 | ||
| const dataName1 = 'KeyData1'; | ||
| const dataName2 = 'KeyData2'; | ||
| const inserted = await insertDataClassData([ |
There was a problem hiding this comment.
nit: This could be flaky if this is the first test run on a new database (as it was for me locally 😄). Consider using -1.
2 participants
Rationale
In preparation for work to consolidate dataclass update methods (from the current single _update vs DIB based batch update), we need to make some changes to the current dataclass schema.
This PR does not drop LSID from provisioned, nor does it deprecate support of update using LSID.
Related Pull Requests
Changes